-
Notifications
You must be signed in to change notification settings - Fork 41
core/desktopentry: add file system watcher for desktop entry directories #114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
I'm generally conflicted on this one because I prefer state change notifications to manually triggered scanning, which often leads to polling.
Looks like its just a simple directory watch, but it likely has to be recursive on every potential desktop entry dir, otherwise it's unlikely to handle nix generation switches and such. This is the general direction I'd like to take it in. No pressure on you to implement it if you don't want to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you've marked it as WIP. Partial review
@outfoxxed I addressed the comments, I had it initially watching .desktop files and then decided against it, to just watch the directories and rescan when there's a change instead. Sorry for the confusion, not wip anymore - ready for review |
Would there be anything holding this one up for primetime? It will be quite useful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional notes:
- Please don't add useless comments
- Please do scanning triggered by a change in another thread via QRunnable. The desktop entry scanner takes long enough to run that it may have a noticeable stutter on some systems without it.
Thanks for taking another look, I implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still doesn't appear to handle anything above the applications folder in the path being swapped out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this one has required a lot of re-review. Hopefully we're near done.
for (const auto& subdir: subdirs) this->watcher.addPath(subdir.absoluteFilePath()); | ||
} | ||
|
||
void DesktopEntryMonitor::onDirectoryChanged(const QString&) { this->debounceTimer.start(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately once we include parents it becomes more complex than this. Ideally we don't want to trigger a rescan if a user creates a file in / or something. However this is acceptable for now because fixing it properly will require rewriting QFileSystemWatcher.
Think I addressed all of the ones from this round @outfoxxed hopefully good now 🤞 |
missed the collapsed ones? |
yep, just addressed them - hopefully good now for real 🤦 |
De-dupe code paths to scanAndWatch function, Optimize lookups by using a QSet, remove paths from watcher if deleted
- Create less fs watchers - Just re-scan when a directory containing .desktop files changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've received a report that this patch also duplicates the desktop entry list in rare cases, though I didn't see anything that would obviously cause that.
Apologies for the long review turnaround as well, I've been away for the last couple weeks.
Haven't seen the entries get duplicated myself , addressed the latest round of comments |
DesktopEntries only scans one time~~, and doesnt expose scanning functions to QML. This adds a
rescan
function, a signal for applications changed. It just allows a nicer launcher that doesn't get stale.~~This adds:
DesktopUtils
which had a common function for locating.desktop
filesDesktopEntryMonitor
which implements QFileSystemWatcherDesktopEntryManager
and add a new signalapplicationsChanged